Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚧 WIP [next] feat!: refactor stacks.js #1596

Closed
wants to merge 13 commits into from
Closed

Conversation

janniks
Copy link
Collaborator

@janniks janniks commented Nov 13, 2023

💡 To reduce reviewing difficulty some PRs will merge into this branch. Also no real "logic" is changed here, to keep it simple.

High-level changes:

  • Move to new @stacks/api package (main way to do networking for users)
  • Move to static "network" objects (simpler way to specify opts for methods)

Screenshot 2023-12-18 at 16 20 39

Refactor 🚂

  • Removes @stacks/network package
    • Moves most functionality to transactions package (this will become the main package allowing most folks to use it and not need to worry about other imports)
    • Move to a network OBJECT approach (similar to bitcoin-js, micro-btc-signer, and others). The network will not be an instantiated class, but a static exported object with all information that might be relevant (txVersion, magicBytes, etc.)
    • Remove fetch functions from network and introduce StacksNodeApi, which can derive it's URL from a network, but should be treated separately -- more closely mirroring to what's actually happening in the background.
    • The majority of users doesn't realize network is also setting changes for "networking"
      • Most users don't customize fetchFn or URL. For users hosting their own node the only change becomes { api: { url: "my-node.com" }, ...} in the params of most relevant tx functions.
  • Removes duplicate/unnecessary definitions from @stacks/common

Some changes might irritate users at first, but I think it's an approach that follows the structure of the blockchain and in a way educated while using the library. More to follow

Curious to get feedback 🙏 @hugocaillard @zone117x @rafaelcr

Open Questions:

  • Thoughts on param naming?
    • Should url be baseUrl, or nodeUrl?
    • Should StacksApi be StacksNode or StacksNodeApi
      • I like it short, but "node" could be helpful (even though most users wrap their node with the API). We could also add a "node-only mode" which ensures the requests it's trying to make exist on the v2 rather than extended.
  • What should live where?
    • Should we move most things into @stacks/transactions (is there a case for having users import from @stacks/common for basic interactions or should it stay for helpers used by all @stacks/ packages?) common will be non-Stacks things, transactions will be the main package ✅

Examples

Diff for most methods

const transaction = await makeSTXTokenTransfer({
  recipient,
  amount,
  senderKey,
  memo,
- network: new StacksTestnet({ url: "mynode-optional.com" }), // optional options
+ network: STACKS_TESTNET,
+ api: { url: "mynode-optional.com" } // optional param
});

String enum params still work (e.g. 'testnet') ☝️

New API class

const api = new StacksNodeApi({ network: STACKS_TESTNET });
await api.broadcastTransaction(tx); // bundled client, discoverable API maintained endpoints/methods (with helper processing/decoding)

Copy link

vercel bot commented Nov 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
stacksjs-docs ❌ Failed (Inspect) Dec 18, 2023 3:42pm

@janniks janniks force-pushed the feat/add-api-package branch from d6c09f9 to 4e6c8b3 Compare November 13, 2023 17:30
@janniks janniks changed the title [next] feat: add api package [next] feat: add api class Nov 13, 2023
BREAKING CHANGE:
Convert `version` of PoXAddress from array to number.
@janniks janniks changed the title [next] feat: add api class [next] feat: remove network package Nov 16, 2023
@janniks
Copy link
Collaborator Author

janniks commented Dec 4, 2023

Any other thoughts on this change? @zone117x
I still don't like the separation of common+transactions. It's super unclear when something should be in which.
Currently common houses things like TransactionVersion/ChainID which is very transactiony. 😅

Screenshot 2023-12-04 at 11 32 25
For dependents it doesn't really matter, since pretty much everything already imports from transaction

A) Do you think we could remove the package and move its contents to transactions?
B) Or maybe move most things to transactions, and leave common for very pure "helpers" (e.g. buffer, bytes, hex).

I'm thinking if most people also need to do text<>hex conversion or similar, we might as well do everything in transactions, so folks only need one dependency for 95% of use-cases.

@zone117x
Copy link
Member

zone117x commented Dec 4, 2023

Hmm good arguments for both options. I don't really have a strong opinion. Kinda lean towards keeping the common package so that other packages aren't transitively dependent on the transaction package for helpers that are unrelated to transactions

@janniks janniks changed the title [next] feat: remove network package [next] feat!: refactor stacks.js Jan 29, 2024
@janniks janniks changed the title [next] feat!: refactor stacks.js 🚧 WIP [next] feat!: refactor stacks.js Jan 29, 2024
@janniks
Copy link
Collaborator Author

janniks commented Jan 29, 2024

Splitting this out into separate PRs

@janniks
Copy link
Collaborator Author

janniks commented Feb 5, 2024

closed in favor of #1622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants